SubMesh::RecalculateNormals improvement #609

ntfshard · 2024-05-12T18:12:12Z

🦟 Bug fix

Fixes #N/A

Summary

Previous version of RecalculateNormals member function has a O(n^2) complexity because inner loop where we are looking for normal elements. But we already know indexes of this elements. So we can get rid of inner loop.
And seems we should check indices.size() < 3u || indices.size() % 3u != 0 due to if it will be 4, main loop will start and on second iteration it will start reading out of array.
Also seems condition in the begin should check amount of indexes/vector, but not normals, due to we are resizing it later.
Maybe this function also should have a check of enum PrimitiveType of data, seems attempt to call it for POINTS and LINES is not legit (for rest 4 types maybe it's ok, not sure)

Checklist

Signed all commits for DCO
Added tests
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

ntfshard · 2024-05-13T15:36:25Z

I force-pushed commit due to previous one was not signed and I made boundary check more strict

iche033 · 2024-05-13T17:55:44Z

looks like there is still an issue with DCO, can you try fix it again?

iche033 · 2024-05-14T20:58:04Z

I was testing this with a capsule shape and I noticed that a seam is now visible after the changes:

Before:

After:

The RecaculateNormals function was ported from gazebo-classic so I went through it's history and found this:
gazebosim/gazebo-classic@ea54b56

From the commit msg, the loop was actually implemented on purpose to make objects appear smoother. The effect of that loop is that more normals are updated instead of limiting to 3 normals. Not efficient but produces smoother results. I'm leaning towards keeping the current code as it was intentional.

ntfshard · 2024-05-14T23:57:17Z

I see. Thank you for testing and clarification. The main problem with current code that it could work about a minute if polygon has quite a lot of triangles even on a modern hardware. Maybe I can try to re-implement initial behaviour with unordered_map. I guess it should be ok to exchange execution time to a memory consumption spike

iche033 · 2024-05-15T00:05:09Z

Maybe I can try to re-implement initial behaviour with unordered_map. I guess it should be ok to exchange execution time to a memory consumption spike

yes that would be good to try, thanks

Signed-off-by: Maksim Derbasov <[email protected]>

graphics/src/SubMesh.cc

Co-authored-by: Dmitry Skorobogaty <[email protected]> Signed-off-by: Maksim Derbasov <[email protected]>

Signed-off-by: Maksim Derbasov <[email protected]>

ntfshard · 2024-07-20T19:14:53Z

Sorry for a delay. Here is an another solution. We are grouping all points by x coordinate and check only points with a same x coordinate (and adjacent groups, due to operator== for Vector3d is actually treat equal a coordinate in a plus/minus Epsilon range.)

For a relatively small sub-meshes it works slower(on my hardware, around 8 milliseconds) than previous one. It looks like a minimal running time constant. Not sure how critical is it. If it's a showstopper, I can dispatch algorithms according to a input size, but it will make code more complex(for a future readers), that's why I came with this version first.

~~Also not sure what with Jenkins CI, I tried to run this binaries locally and they seems working fine~~
UPD: seems no problem, I'm just a stupid

And the last thing, I don't have a full gazebo setup in current environment, can rely only on unit tests (w/o visual manual check)

Signed-off-by: Maksim Derbasov <[email protected]>

ntfshard · 2024-07-23T09:01:02Z

graphics/src/SubMesh.cc

-  if (this->dataPtr->normals.size() < 3u)
+  if (this->dataPtr->indices.empty()
+      || this->dataPtr->indices.size() % 3u != 0)
    return;



It feels like it also should check: || this->dataPtr->primitiveType != SubMesh::TRIANGLES but not 100% sure that it's only one primitiveType

+1, I think it's good to be explicit and check that it's a TRIANGLES primitive type. I think other primitive types don't quite work yet but this is just in case better support is implemented for other types in the future.

iche033

The new approach looks fine to me. I tested the code and the capsule renders fine without the seam. Just one minor comment on adding some doc to explain the logic.

iche033 · 2024-07-23T17:55:43Z

graphics/src/SubMesh.cc

-  if (this->dataPtr->normals.size() < 3u)
+  if (this->dataPtr->indices.empty()
+      || this->dataPtr->indices.size() % 3u != 0)
    return;



+1, I think it's good to be explicit and check that it's a TRIANGLES primitive type. I think other primitive types don't quite work yet but this is just in case better support is implemented for other types in the future.

iche033 · 2024-07-23T18:04:26Z

graphics/src/SubMesh.cc

+  }
+
+  template<typename Visitor>
+  void Visit(const gz::math::Vector3d &_point, Visitor _v) const


can you add some documentation to explain the logic in this function? I think it's to find and include all indices of vertices that are within the tolerance of the math::equal checks, so that their normals all count towards the final calculation?

Signed-off-by: Maksim Derbasov <[email protected]>

iche033

looks good, thanks for iterating

ntfshard requested a review from marcoag as a code owner May 12, 2024 18:12

github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels May 12, 2024

ahcorde assigned ntfshard May 13, 2024

ntfshard force-pushed the submesh-normals branch from a27e8e0 to de01bba Compare May 13, 2024 15:33

ntfshard closed this May 13, 2024

ntfshard reopened this May 13, 2024

ntfshard force-pushed the submesh-normals branch from de01bba to 2626310 Compare May 13, 2024 23:49

ntfshard force-pushed the submesh-normals branch from 2626310 to 5e36661 Compare July 20, 2024 10:51

ntfshard added 5 commits July 20, 2024 19:56

SubMesh::RecalculateNormals improvement

1ca2962

Signed-off-by: Maksim Derbasov <[email protected]>

Added test, reverted lib code

66c69c7

Signed-off-by: Maksim Derbasov <[email protected]>

Typos fix

3882da1

Signed-off-by: Maksim Derbasov <[email protected]>

Group vertexes by X

3ea5134

Signed-off-by: Maksim Derbasov <[email protected]>

Clean-up

e22e6ff

Signed-off-by: Maksim Derbasov <[email protected]>

ntfshard force-pushed the submesh-normals branch from 5e36661 to e22e6ff Compare July 20, 2024 10:56

skorobogatydmitry reviewed Jul 20, 2024

View reviewed changes

graphics/src/SubMesh.cc Outdated Show resolved Hide resolved

ntfshard and others added 2 commits July 20, 2024 23:55

Update comment in graphics/src/SubMesh.cc

b0605b8

Co-authored-by: Dmitry Skorobogaty <[email protected]> Signed-off-by: Maksim Derbasov <[email protected]>

Const tweak

0ba046a

Signed-off-by: Maksim Derbasov <[email protected]>

ntfshard force-pushed the submesh-normals branch from ccfcb2d to 0ba046a Compare July 20, 2024 18:26

azeey requested a review from iche033 July 22, 2024 18:59

ntfshard added 2 commits July 23, 2024 10:22

Fixing out of border access

fe8b008

Signed-off-by: Maksim Derbasov <[email protected]>

Fix warning in test

833962c

Signed-off-by: Maksim Derbasov <[email protected]>

ntfshard commented Jul 23, 2024

View reviewed changes

iche033 reviewed Jul 23, 2024

View reviewed changes

:Iteration

cd01fa6

Signed-off-by: Maksim Derbasov <[email protected]>

ntfshard force-pushed the submesh-normals branch from 4feb0b3 to cd01fa6 Compare July 24, 2024 16:32

ntfshard requested a review from iche033 July 25, 2024 03:00

iche033 approved these changes Jul 25, 2024

View reviewed changes

iche033 merged commit ad1d5ad into gazebosim:gz-common5 Jul 25, 2024
12 checks passed

ntfshard deleted the submesh-normals branch July 25, 2024 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SubMesh::RecalculateNormals improvement #609

SubMesh::RecalculateNormals improvement #609

ntfshard commented May 12, 2024 •

edited

Loading

ntfshard commented May 13, 2024

iche033 commented May 13, 2024

iche033 commented May 14, 2024 •

edited

Loading

ntfshard commented May 14, 2024

iche033 commented May 15, 2024

ntfshard commented Jul 20, 2024 •

edited

Loading

ntfshard Jul 23, 2024 •

edited

Loading

iche033 Jul 23, 2024

iche033 left a comment

iche033 Jul 23, 2024

iche033 Jul 23, 2024

iche033 left a comment

SubMesh::RecalculateNormals improvement #609

SubMesh::RecalculateNormals improvement #609

Conversation

ntfshard commented May 12, 2024 • edited Loading

🦟 Bug fix

Summary

Checklist

ntfshard commented May 13, 2024

iche033 commented May 13, 2024

iche033 commented May 14, 2024 • edited Loading

ntfshard commented May 14, 2024

iche033 commented May 15, 2024

ntfshard commented Jul 20, 2024 • edited Loading

ntfshard Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

iche033 Jul 23, 2024

Choose a reason for hiding this comment

iche033 left a comment

Choose a reason for hiding this comment

iche033 Jul 23, 2024

Choose a reason for hiding this comment

iche033 Jul 23, 2024

Choose a reason for hiding this comment

iche033 left a comment

Choose a reason for hiding this comment

ntfshard commented May 12, 2024 •

edited

Loading

iche033 commented May 14, 2024 •

edited

Loading

ntfshard commented Jul 20, 2024 •

edited

Loading

ntfshard Jul 23, 2024 •

edited

Loading